[v8] upgrade module to v8 standard go version workflow #3256
Conversation
jcvrabo
changed the title
|
Hi, would like to know what's the status with reviewing these prs, the longer it takes the harder it will be to maintain it. I was updating it with the repository updates, until it started breaking the tests and I noticed that PRs were approved that themselves were breaking the tests. The V2 api is getting phased out, and we have been testing that (disabling the v2 api) which ends up failing pretty much all plugins. These PRs became urgent, I would say, if you want to keep on supporting plugins... either that or we all have to start developing plugins without go modules... which is also not acceptable I don't think I need to refer you to the v2 phasing out, but just in case https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0032-cfapiv2-eol.md I will later try to resync the PRs with the head of the branches, but would still like to know what's the ETA for an approval here, as I also won't be fixing tests broken at the head of the branches, which will have the PR ending up with failed checks. |
# Conflicts: # cf/commands/application/apps.go # command/v7/create_app_command.go # command/v7/create_app_command_test.go # command/v7/create_user_provided_service_command.go # command/v7/create_user_provided_service_command_test.go # command/v7/push_command.go # go.mod # integration/v7/isolated/app_command_test.go # integration/v7/isolated/create_app_command_test.go
|
Following comment applies to all PR's related to updating the project to follow the standard go versioning workflow The initial hack based on using the previous non-module cli version that the cfnetworking project (archived) was using, was expected to break sooner or later. It broke sooner for the main and v8 branches as some projects had updates with non-backward compatible versions which basically broke the builds. I have now copied (and made some small updates) the cfnetworking project into the api package. It was already followin pretty much the same implementation of the other api packages, so even though I don't exactly agree with the approach, I kept it to keep it coherent. Apparently there was also another merged PR that broke the cf restage timeout test with rolling strategy after adding the --max-in-flight. I split it it two tests, one with the flag and one without, as the end result is different. I basically fixed it so that I could have the PR's green with the integration tests. I would like to reiterate my plea for a review, as while the pr's are not complex they do touch a lot of files and consequentially will almost always end up having a conflicted status for every other merged PR. I do try to maintain the forked branches with upstream updates, but that also takes time and effort to do so, when the PR's are non-trivial. |
# Conflicts: # actor/v7action/revisions_test.go # api/cloudcontroller/ccv3/revisions_test.go # command/v7/revision_command.go # command/v7/revision_command_test.go # integration/v7/isolated/revision_command_test.go
# Conflicts: # command/v7/create_route_command.go # command/v7/create_route_command_test.go # command/v7/map_route_command.go # command/v7/route_command.go # go.mod # resources/droplet_resource.go
|
As far as I understood, this change must be merged in order to allow this go project to be used as dependency in other go modules, right? If the answer of previous question is yes, why this change is not merged already, it's blocking all cf cli plugin maintainers to use latest v3 APIs models. When this change will be merged or what are the requirements to merge? |
|
The answer is yes to your question, but since I haven't gotten any feedback for some time, I stopped maintaining the fork and it now runs in conflict. When I get some positive feedback I'll update the fork with the latest commits and resolve the conflicts |
# Conflicts: # actor/pluginaction/install.go # actor/v7action/deployment_test.go # actor/v7action/revisions_test.go # actor/v7pushaction/create_deployment_for_push_plan_test.go # actor/v7pushaction/handle_disk_override.go # actor/v7pushaction/handle_disk_override_test.go # actor/v7pushaction/handle_instances_override.go # actor/v7pushaction/handle_instances_override_test.go # actor/v7pushaction/handle_log_rate_limit_override.go # actor/v7pushaction/handle_log_rate_limit_override_test.go # actor/v7pushaction/handle_memory_override.go # actor/v7pushaction/handle_memory_override_test.go # actor/v7pushaction/setup_deployment_information_for_push_plan.go # actor/v7pushaction/setup_deployment_information_for_push_plan_test.go # api/cloudcontroller/ccv3/buildpack_test.go # api/cloudcontroller/ccv3/deployment_test.go # api/cloudcontroller/ccv3/droplet_test.go # api/cloudcontroller/ccv3/info.go # api/cloudcontroller/ccv3/manifest_test.go # api/cloudcontroller/ccv3/organization_test.go # api/cloudcontroller/ccv3/paginated_resources_test.go # api/cloudcontroller/ccv3/requester_test.go # api/cloudcontroller/ccv3/space_test.go # api/uaa/prompts_test.go # api/uaa/version_test.go # cf/actors/push.go # cf/net/warnings_collector.go # command/v7/buildpacks_command.go # command/v7/buildpacks_command_test.go # command/v7/config_command_test.go # command/v7/copy_source_command.go # command/v7/create_buildpack_command.go # command/v7/delete_buildpack_command.go # command/v7/delete_buildpack_command_test.go # command/v7/restage_command.go # command/v7/restart_command.go # command/v7/revisions_command_test.go # command/v7/rollback_command.go # command/v7/ssh_code_command_test.go # command/v7/update_buildpack_command.go # command/v7/v7fakes/fake_set_label_actor.go # go.mod # go.sum # integration/shared/isolated/logs_command_test.go # integration/v7/isolated/restage_command_test.go # resources/deployment_resource.go # util/configv3/json_config_test.go # util/configv3/load_config_test.go
|
tests seem to have failed due to a dns timeout, perhaps the tests could run again |
|
This PR is so large that it's difficult to review natively in GitHub, so I'll post some thoughts here. I'm generally supportive of properly-modularizing the cf CLI, even if it's still not a supported usage. cfnetworking-cli-apiThis PR's solution appears to be vendoring and modifying the repo: 65e6e41 As another option, we could write an adapter shim. For example: https://github.com/cloudfoundry/cli/blob/gerg-v8-mod/command/v7/shared/new_networking_client.go#L12-L27 In the long run, I think we should still remove the dependency on the ancient, archived module, but we could theoretically break that out as an independent change. Breaking ChangeI go back and forth on whether properly-modularizing cf v8 is an acceptable breaking change:
The more aggressive option would be to make the change in v8, since it was the correct thing to do for go module major versions in the first place. Anyone consuming the cf CLI as a module is already kinda off-spec, so they could probably roll with the punches (or stay pinned to their current sha). The more conservative option would be to cut a v9 that is just a properly-modularized v8, with no other breaking changes. |
|
You are right in most accounts @Gerg Yes I'm putting back the cfnetworking library into the project (I'm saying putting back because when I look at cfnetworking I have the distinct impression it lived in the cf cli project in the beginning, it has the exact same structure, if it wasn't there... then indeed, I'm just vendoring it :) ) Yes, I totally agree that the cf networking client should be rewritten, or have that part simply using https://github.com/cloudfoundry/policy_client , for example. But I am basically focusing in modularizing the cli, I have always presented this as a transient change and all other structural changes would be brought afterwards (that would also require a more in-depth code/functionality analysis which I didn't do). The reason why this move was done as part of the modularization process is that cfnetworking is not only archived but it implements a plain anti-pattern of cyclic dependencies, so... cfnetworking has cf cli as a dependency, and that means that if you try to modularize v8 there will be a conflict in some cli packages using v8 and others using it without... so... the simpler solution was to move cfnetworking to cf cli and have its dependencies set to in-project (as I think it was in the beginning). I do contest the concept of "breaking change", although it kind of is, the breaking part only affects the project itself and it would "break" all existing prs which would have to be updated. This is because either you're developing non-modularized plugins which could be affected by it (haven't really tested that) but I would expect most newer plugins to already be modularized and stuck on the incompatible 7.10 version... updating the module would not affect those plugins and when they would update to v8, it would be a straightforward update. Like you mention... cli is just not currently consumable as a module, so... no-one is in that scenario to be affected (and the versioning workflow also prevents that, as you have to update your version to v8 in order to consume it anyway), and updating should be straightforward. I don't know when v9 is planned to come out, I'm fine with either option (updating v8 or just v9 and having it as a live version, a pr for main is also available), but regardless... this update will always be a big-bang approach, you can't do it in parts, and it's better to vendor code and keep legacy structure to have less changes to review... in summary... this basically updates all imports to start using v8 and changes cfnetworking dependency to a vendored one, it's a large pr but with little functional content. Thanks for taking a look |
3 participants
Thank you for contributing to the CF CLI! Please read the following:
If your contribution falls under a company CLA but your membership is not public, expect delays while we confirm.
Note: Please create separate PR for every branch (main, v8 and v7) as needed.
Description of the Change
This change upgrades the module name according to the new go version workflow as described in https://go.dev/doc/modules/release-workflow
It also has a small fix in the app_summary_displayer_test.go date regex so that it supports up to 4 character time zone abbreviations
The fix is presented as a transitional one, the cli has dependency to the archived project https://github.com/cloudfoundry-incubator/cfnetworking-cli-api which doesn't use go modules and has a recursive dependency back to the cli. This makes it difficult to smoothly upgrade it in the future. The project now has a dependency of the latest vli version without modules (v7.1.0) which is used by the UAAClient to also create a legacy connection and then use it when creating the cf networking client. It's less than optimal.
The way forward is to remove the recursive dependency by either unarchiving the cfnetworking project (and removing the recursive dependency), creating a new library or adding the cf networking client directly to the cli code base. This workaround is a quick fix to move forward with standard versioning.
Why Is This PR Valuable?
At the moment the development of plugins with go modules is dependent on cli v7.1.0 (which is still not a module). This implies that plugins cannot be updated with any security and version updates that may come from newer versions.
The PR will allow plugins to be updated to the latest cli version.
Applicable Issues
#2291
How Urgent Is The Change?
It has a relative urgency, if you consider the need to update plugins as well as making this change as soon as possible for further progress of the cli
Other Relevant Parties
To my knowledge this only affects plugins (and their maintainers/developers).